Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

revert: Conn type changes in #10012 and #10061 #10255

Merged
merged 2 commits into from
Apr 20, 2021
Merged

revert: Conn type changes in #10012 and #10061 #10255

merged 2 commits into from
Apr 20, 2021

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Apr 19, 2021

This reverts a8057e3 and 9c7c9a3 which introduced breaking changes.

@lucacasonato
Copy link
Member

lucacasonato commented Apr 19, 2021 •
edited
Loading

It might be breaking again now that Conn is not generic anymore (for 1.9.0 users). Maybe we should keep it being generic, and then just do nothing with the generic parameter?

@kitsonk
Copy link
Contributor Author

kitsonk commented Apr 19, 2021

It might be breaking again now that Conn is not generic anymore (for 1.9.0 users). Maybe we should keep it being generic, and then just do nothing with the generic parameter?

But that will likely be breaking then, because the generic does nothing, meaning some things that were assignable won't be assignable.

lucacasonato
lucacasonato approved these changes Apr 19, 2021
View reviewed changes
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Let's risk it 😄

@piscisaureus
Copy link
Member

Isn't it possible to support multiple signature? Maybe we can keep the new one, but add back support for the old signature?

@lucacasonato
Copy link
Member

Isn't it possible to support multiple signature?

No. This is what a8057e3 attempted (through the use of a default type argument), but this failed. See #10200.

@piscisaureus
Copy link
Member

No. This is what a8057e3 attempted (through the use of a default type argument), but this failed. See #10200.

Ok, ship it then.

@kitsonk kitsonk merged commit b6203cb into denoland:main Apr 20, 2021
@kitsonk kitsonk deleted the revert_conn_changes branch April 20, 2021 00:12
@jzaki jzaki mentioned this pull request Apr 21, 2021
Closed
@kt3k kt3k mentioned this pull request Dec 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucacasonato lucacasonato lucacasonato approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kitsonk @lucacasonato @piscisaureus